CLDSRV754: add GetBucketLogging and PutBucketLogging endpoints #5966

leif-scality · 2025-10-06T17:24:59Z

Add GetBucketLogging and PutBucketLogging endpoints

The package.json changes will be reverted before change

bert-e · 2025-10-06T17:25:03Z

Hello leif-scality,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-10-06T17:25:15Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-754 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.1.5

Please check the Fix Version/s of CLDSRV-754, or the target
branch of this pull request.

codecov · 2025-10-06T17:29:56Z

Codecov Report

❌ Patch coverage is 98.63014% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.82%. Comparing base (735303b) to head (d013306).
⚠️ Report is 3 commits behind head on development/9.1.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
lib/api/bucketPutLogging.js	97.67%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/api/api.js	`91.21% <100.00%> (+0.08%)`	⬆️
lib/api/bucketGetLogging.js	`100.00% <100.00%> (ø)`
lib/api/bucketPutLogging.js	`97.67% <97.67%> (ø)`

... and 1 file with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.1    #5966      +/-   ##
===================================================
+ Coverage            83.72%   83.82%   +0.09%     
===================================================
  Files                  191      193       +2     
  Lines                12233    12306      +73     
===================================================
+ Hits                 10242    10315      +73     
  Misses                1991     1991

Flag	Coverage Δ
ceph-backend-test	`64.79% <95.89%> (+0.18%)`	⬆️
file-ft-tests	`67.10% <95.89%> (+0.17%)`	⬆️
kmip-ft-tests	`27.15% <21.91%> (-0.04%)`	⬇️
mongo-v0-ft-tests	`68.42% <95.89%> (+0.16%)`	⬆️
mongo-v1-ft-tests	`68.41% <95.89%> (+0.16%)`	⬆️
multiple-backend	`34.17% <21.91%> (-0.08%)`	⬇️
sur-tests	`34.71% <21.91%> (-0.11%)`	⬇️
sur-tests-inflights	`36.63% <21.91%> (-0.13%)`	⬇️
unit	`68.55% <98.63%> (+0.22%)`	⬆️
utapi-v2-tests	`33.50% <21.91%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bert-e · 2025-10-06T18:51:48Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-754 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.0.31
9.1.5

Please check the Fix Version/s of CLDSRV-754, or the target
branch of this pull request.

jonathan-gramain · 2025-10-06T23:51:30Z

lib/api/bucketPutLogging.js

+                    return callback(err);
+                }
+
+                return callback(null);


Suggested change

return callback(null);

return callback();

jonathan-gramain · 2025-10-06T23:53:15Z

tests/unit/api/bucketGetLogging.js

+        const request = createGetLoggingRequest(bucketName);
+
+        bucketGetLogging(authInfo, request, log, (err, xml) => {
+            assert.strictEqual(err, null);


Suggested change

assert.strictEqual(err, null);

assert.ifError(err);

replaced the strict equal with ifError in both test files

jonathan-gramain · 2025-10-06T23:54:59Z

tests/unit/api/bucketGetLogging.js

+        const putRequest = createLoggingRequest(bucketName, loggingXML);
+
+        // First enable logging
+        bucketPutLogging(authInfo, putRequest, log, err => {


Optional: you may want to use async.series or async.waterfall to reduce nesting levels

changed to waterfall to handle logging and metrics in only one place

jonathan-gramain · 2025-10-06T23:57:05Z

tests/unit/api/bucketGetLogging.js

+        });
+    });
+
+    it('should handle multiple get requests consistently', done => {


I don't know if this test is really useful, but no problem to keep it if you prefer.

jonathan-gramain · 2025-10-06T23:58:58Z

tests/unit/api/bucketPutLogging.js

+        bucketPutLogging(authInfo, request, log, err => {
+            assert(err);
+            // Missing body should trigger XML parsing error
+            assert(err.is.MalformedXML || err.code === 'MalformedXML');


Is there a reason to support both error types?

no, removed the string check

dvasilas

Looks good.

We should also add monitoring to these APIs:

cloudserver/lib/api/bucketPutACL.js

Line 303 in e2ace24

monitoring.promMetrics('PUT', bucketName, '200', 'bucketPutACL');

leif-scality · 2025-10-07T14:33:51Z

Looks good.

We should also add monitoring to these APIs:

cloudserver/lib/api/bucketPutACL.js

Line 303 in e2ace24

monitoring.promMetrics('PUT', bucketName, '200', 'bucketPutACL');

added metrics

dvasilas · 2025-10-08T09:11:03Z

I think you need to remove logging from unsupportedQueries in constants because currently the API returns NotImplemented:

cloudserver/constants.js

Line 120 in e2ace24

'logging',

dvasilas · 2025-10-08T09:30:43Z

lib/api/bucketPutLogging.js

+        request,
+    };
+
+    return waterfall([


Should we check if the target bucket exists? Do you know if AWS returns an error if it does not exist?

An error occurred (InvalidTargetBucketForLogging) when calling the PutBucketLogging operation: The target bucket for logging does not exist

We can do the same

added a check and functional tests

lib/api/api.js

lib/api/bucketPutLogging.js

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js

BourgoisMickael · 2025-10-14T17:25:35Z

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js

+                _testPutBucketLoggingError(s3, validLoggingConfigWithGrants, 501, 'NotImplemented', done);
+            });
+
+            it('should disable logging with empty BucketLoggingStatus', done => {


It can be difficult to read with many nested callbacks.

I'd prefer if possible now to use async await instead of done callback in tests.

You can use await s3.putBucketLogging({...}).promise().

We started using that on new test files

leif-scality · 2025-10-16T08:43:57Z

/approve

bert-e · 2025-10-16T08:44:02Z

Unknown command

I didn't understand this comment by @leif-scality:

/approce

I don't know what approce means.

Please edit or delete the corresponding comment so I can move on.

leif-scality · 2025-10-16T08:44:10Z

/approve

leif-scality · 2025-10-16T08:54:23Z

ping

leif-scality · 2025-10-16T08:57:38Z

ping

leif-scality · 2025-10-16T09:03:57Z

/status

bert-e · 2025-10-16T09:06:39Z

Status

Status report is not available.

The following options are set: approve

bert-e · 2025-10-16T09:06:50Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-754-bucket-put-get-logging into
development/9.0
w/9.1/improvement/CLDSRV-754-bucket-put-get-logging will be merged into development/9.1

The following branches will NOT be impacted:

development/7.10
development/7.4
development/7.70
development/8.8

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: approve

leif-scality · 2025-10-16T09:08:27Z

/reset

bert-e · 2025-10-16T09:08:36Z

Reset complete

I have successfully deleted this pull request's integration branches.

The following options are set: approve

bert-e · 2025-10-16T09:08:49Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-754-bucket-put-get-logging into
development/9.0
w/9.1/improvement/CLDSRV-754-bucket-put-get-logging will be merged into development/9.1

The following branches will NOT be impacted:

development/7.10
development/7.4
development/7.70
development/8.8

You can set option create_pull_requests if you need me to create
integration pull requests in addition to integration branches, with:

@bert-e create_pull_requests

The following options are set: approve

bert-e · 2025-10-16T09:09:39Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-754 contains:

9.0.31
9.1.5

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.1.5

Please check the Fix Version/s of CLDSRV-754, or the target
branch of this pull request.

The following options are set: approve

leif-scality · 2025-10-16T09:10:24Z

/reset

bert-e · 2025-10-16T09:10:32Z

Reset complete

I have successfully deleted this pull request's integration branches.

The following options are set: approve

leif-scality · 2025-10-16T09:10:44Z

/approve

bert-e · 2025-10-16T09:28:34Z

Build failed

The build for commit did not succeed in branch improvement/CLDSRV-754-bucket-put-get-logging

The following options are set: approve

bert-e · 2025-10-16T09:47:49Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/9.1

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70
development/8.8
development/9.0

Please check the status of the associated issue CLDSRV-754.

Goodbye leif-scality.

The following options are set: approve

tcarmet approved these changes Oct 6, 2025

View reviewed changes

leif-scality changed the base branch from development/9.1 to development/9.0 October 6, 2025 18:51

jonathan-gramain approved these changes Oct 6, 2025

View reviewed changes

dvasilas approved these changes Oct 7, 2025

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 856c74c to 3402063 Compare October 7, 2025 17:43

dvasilas reviewed Oct 8, 2025

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch 5 times, most recently from 951dd9c to 28ee88f Compare October 13, 2025 17:02

BourgoisMickael reviewed Oct 13, 2025

View reviewed changes

lib/api/api.js Outdated Show resolved Hide resolved

BourgoisMickael reviewed Oct 13, 2025

View reviewed changes

lib/api/api.js Outdated Show resolved Hide resolved

BourgoisMickael reviewed Oct 13, 2025

View reviewed changes

lib/api/bucketPutLogging.js Outdated Show resolved Hide resolved

BourgoisMickael reviewed Oct 13, 2025

View reviewed changes

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js Outdated Show resolved Hide resolved

BourgoisMickael reviewed Oct 13, 2025

View reviewed changes

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js Outdated Show resolved Hide resolved

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 029a87c to b08e0a2 Compare October 14, 2025 16:39

BourgoisMickael reviewed Oct 14, 2025

View reviewed changes

tests/functional/aws-node-sdk/test/bucket/putBucketLogging.js Outdated Show resolved Hide resolved

BourgoisMickael reviewed Oct 14, 2025

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from b08e0a2 to e7c5059 Compare October 15, 2025 16:38

BourgoisMickael approved these changes Oct 15, 2025

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 5dc0b41 to 689aa68 Compare October 16, 2025 08:06

leif-scality added 3 commits October 16, 2025 11:08

CLDSRV-754: add GetBucketLogging and PutBucketLogging endpoints

e2d0ba5

CLDSRV-754: add s3-node-sdk functional tests

57bf772

CLDSRV-754: bump arsenal version to 8.2.33

d013306

leif-scality changed the base branch from development/9.0 to development/9.1 October 16, 2025 09:09

leif-scality force-pushed the improvement/CLDSRV-754-bucket-put-get-logging branch from 689aa68 to d013306 Compare October 16, 2025 09:09

bert-e merged commit d013306 into development/9.1 Oct 16, 2025
52 of 53 checks passed

bert-e deleted the improvement/CLDSRV-754-bucket-put-get-logging branch October 16, 2025 09:47

CLDSRV754: add GetBucketLogging and PutBucketLogging endpoints #5966

CLDSRV754: add GetBucketLogging and PutBucketLogging endpoints #5966

Uh oh!

Conversation

leif-scality commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Oct 6, 2025

Hello leif-scality,

Uh oh!

bert-e commented Oct 6, 2025

Incorrect fix version

Uh oh!

codecov bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bert-e commented Oct 6, 2025

Incorrect fix version

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvasilas left a comment

Choose a reason for hiding this comment

Uh oh!

leif-scality commented Oct 7, 2025

Uh oh!

dvasilas commented Oct 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BourgoisMickael Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leif-scality commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Oct 16, 2025

Unknown command

Uh oh!

leif-scality commented Oct 16, 2025

Uh oh!

leif-scality commented Oct 16, 2025

Uh oh!

leif-scality commented Oct 16, 2025

Uh oh!

leif-scality commented Oct 16, 2025

Uh oh!

bert-e commented Oct 16, 2025

leif-scality commented Oct 6, 2025 •

edited

Loading

codecov bot commented Oct 6, 2025 •

edited

Loading

BourgoisMickael Oct 14, 2025 •

edited

Loading

leif-scality commented Oct 16, 2025 •

edited

Loading